`ot_i2c_transport`: implement device for external I2C communication #190

ziuziakowska · 2025-09-18T15:27:50Z

This is an I2C device that allows external software such as opentitanlib to communicate with and issue commands to devices on a QEMU I2C bus. The communication is done through a chardev.

See docs/i2c_transport.md for usage and protocol information.

The device can be instantiated with these command line parameters:

-chardev pty,id=i2ctp -device ot-i2c-transport,bus=<bus>,chardev=i2ctp, to instantiate on an I2C bus.

See also lowRISC/opentitan#28450 for an implementation and test harness using this device.

hw/opentitan/ot_i2c_transport.c

include/hw/opentitan/ot_i2c_transport.h

docs/opentitan/i2c_transport.md

hw/opentitan/ot_i2c_transport.c

hw/opentitan/ot_i2c.c

hw/opentitan/ot_i2c_transport.c

docs/opentitan/i2c_transport.md

ziuziakowska · 2025-10-09T14:43:44Z

I've made the wire protocol use raw bytes now instead of hex encoded ones, as I now have a functional implementation to talk to this in opentitanlib.

I have also made the device throttle processing using a timer to allow control to be returned to the vCPU so that it can observe the transfers taking place and prepare response bytes. I thought to do this manually in opentitanlib, but after implementing it there I think it is better to have it done in the device in QEMU so that it is transparent to all users of it.

rivos-eblot

There is something I deeply do not understand with how the I2C stack is managed, but I do not want to delay this PR any further, as I guess the feature is needed.

I'll try to find some time to dig deeper.

hw/opentitan/ot_i2c_transport.c

rivos-eblot · 2025-10-09T16:07:48Z

nit pick: do not forget to update the format of the PR title

rivos-eblot · 2025-10-10T15:20:19Z

I had a very quick look at the I2C target mode documentation, and I think the QEMUTimer is not required, as the target implements clock stretching. I think what is required is that the ACK/NACK (controller to target write) or payload (controller from target read) should only be send when the respective HW FIFO are in the proper state. If the FIFO is full (write) no ACK/NACK should be reported (and obviously not data should be send if the FIFO is empty (read).

It looks like everything can be managed with the communication "transport" protocol, and I'm still not sure it covers all use cases and stick to the actual I2C implementation, such as:

SCL is only managed by the controller, the target only obey to it: this means the controller (remote peer) should ask for each byte it wants to read. This gives flow control at byte level, exactly like the I2C physical bus
SCL stretching which is handled by the target would be trivially easy to implement: on poll from the controller to receive one new byte, no reply should be pushed back till SCL stretching is released
ACK/NACK from the controller to resume a read request should be handled as well (can be mixed with its "poll" query if that makes sense).

If the remote host performs an illegal request (e.g. pushing a new byte whereas it has not received a ACK or NACK), the state machine should be moved to ERROR, and only restarted on a new connection for example.

To sum up, I keep thinking it would help to draft some kind of chronogram to compare the I2C physical bus signalling vs. its translation into byte exchange over the comm. socket, which should cover all standard cases.

docs/opentitan/i2c_transport.md

ziuziakowska · 2025-10-13T11:21:20Z

I had a very quick look at the I2C target mode documentation, and I think the QEMUTimer is not required, as the target implements clock stretching. I think what is required is that the ACK/NACK (controller to target write) or payload (controller from target read) should only be send when the respective HW FIFO are in the proper state. If the FIFO is full (write) no ACK/NACK should be reported (and obviously not data should be send if the FIFO is empty (read).

This would be ideal, however clock stretching is unimplemented in I2C, as QEMU's I2C model is quite simplistic and I2C operations cannot be deferred until some data ready condition (e.g I2CSlaveClass i2c_recv must return something as the read data byte, and there is no way to express failure).

As a workaround, the timer is used to throttle the I/O to yield control back to the OT I2C, so that it can observe the transactions going on. Otherwise read transfers will all complete immediately on the I/O thread when it decides to process the I2C transfer, and at no point will OT I2C be aware a read transfer is taking place and have no opportunity to fill the TX FIFO.

SCL is only managed by the controller, the target only obey to it: this means the controller (remote peer) should ask for each byte it wants to read. This gives flow control at byte level, exactly like the I2C physical bus

This is already the case for transfers in both directions - once a read transfer is started, the controller issues a read by sending an R byte, and in any transfer the bytes can be sent with any amount of delay.

At some point I would like to implement a better I2C layer to model this better, there are already some wrappers such as IbexIRQ. For now I have avoided modifying core QEMU code and tried to work around it.

rivos-eblot · 2025-10-13T11:39:16Z

Yes you are right, the I2C API is far too limited, we will need to extend it at some point.

rivos-eblot

Can you open an issue for each open point so this PR can be merged but all the remaining issues are addressed? Please add the link to the comment before resolving it, so it's easier to track the resolution. Thanks.

rivos-eblot · 2025-10-14T09:29:19Z

docs/opentitan/i2c_transport.md

+
+## Protocol
+
+- The protocol over the chardev mirrors the I2C specification closely.


There is a discrepancy with the I2C protocol: when reading, the controller is supposed return a ACK/NACK to the slave to inform it whether it wants to keep reading or not.

Note that in I2C spec:

the START bit is referred as S, while

the STOP bit is referred to as P

I really keep thinking some kind of ASCII art would help understanding the CharDev based protocol vs. the physical protocol

There is a discrepancy with the I2C protocol: when reading, the controller is supposed return a ACK/NACK to the slave to inform it whether it wants to keep reading or not.

Over the protocol, during a read transfer an R byte is sent to read the next byte from the slave, or the transaction could be ended. This would roughly correspond to "ACK and read next byte" and "NACK and stop condition".

Note that in I2C spec:

* the START bit is referred as `S`, while * the STOP bit is referred to as `P`

I think I will rename them then.

docs/opentitan/i2c_transport.md

rivos-eblot · 2025-10-14T09:31:48Z

docs/opentitan/i2c_transport.md

+Where `bus` is any of the system's I2C buses. On earlgrey for example, these are `ot-i2c0`,
+`ot-i2c1`, and `ot-i2c2`.
+
+## Protocol


I think the protocol is missing a version info. It is really important since the CharDev peer implementation is likely to be part of another repository, which means no way to ensure the same protocol version is used on both sides.

There should be an initial handshake to ensure both peers can exchange knowledgeable data.

Is a single byte containing the version before initiating every transaction sufficient?

I think it should be enough to perform it when a new connection is open (that is likely translating into another state in your FSM). When a connection is open, wait for some version info (with a prefix as for the other commands, maybe V?) and a number. I do not think it is worth having some kind of major/minor version split.

Are chardevs in QEMU guaranteed to have at most one active connection? Theoretically socket type chardevs could have many active connections at once. Opentitan tooling will always use pty type, but other users may use something else so it might be something to worry about.

If at most one active connection can exist, then I can do that with another state in the FSM.

Yes, CharDev is either not connected or connected to a single client.
Note: on our side, I think we never use ptys, only unix and TCP sockets.

hw/opentitan/ot_i2c_transport.c

rivos-eblot · 2025-10-14T09:36:14Z

hw/opentitan/ot_i2c_transport.c

+    s->is_read_command = false;
+    s->address = 0u;
+    s->stall = false;
+    s->stall_timer =


Timer is not cancelled on reset, this will bite hard at some point.

I think this is not that bad if we just use the timer (and not the additional stall attribute) as I suggest in a separate comment, since the stall time is so short and we can then avoid timers causing state change across resets. But yes, maybe this should be documented as part of the lack of resets as discussed in other comments.

AlexJones0

Broadly I agree with previous comments on documenting the deficiencies of QEMU's I2C protocol, so that these issues are captured somewhere for reference in the future if more accurate I2C emulation is desired. I've also left a few separate comments.

AlexJones0 · 2025-10-14T09:57:50Z

hw/opentitan/ot_i2c_transport.c

@@ -0,0 +1,318 @@
+/*
+ * OpenTitanTool I2C transport device for OpenTitan


Nit: this appears in many places, but I think it would be good if the QEMU model didn't reference OpenTitanTool/"transport"s at all. While it is what we use for testing upstream OpenTitan, it technically is just host-side SW responsible for interacting with OT and could be replaced with any working host SW stack.

If my understanding is correct, any host SW / user can interact with this device via the protocol, and so it doesn't make sense to specifically label "OpenTitanTool". Maybe "I2C host proxy device" or something similar would be more suitable?

I think "I2C host proxy device" or "I2C proxy device" could work as a name. "transport" was chosen as the immediate use-case for this was usage in opentitanlib, so it made sense to me to call the device after that, but I think now the term is overloaded and confusing.

AlexJones0 · 2025-10-14T10:03:12Z

include/hw/opentitan/ot_i2c_transport.h

+ * THE SOFTWARE.
+ */
+
+#ifndef HW_OPENTITAN_OT_I2C_TRANSPORT_H


Nit: as with above, maybe this might be better as something like HW_OT_HOST_I2C_PROXY_H? etc.

I think the convention is to follow the exact file path, so that would be HW_OPENTITAN_OT_I2C_HOST_PROXY_H for hw/opentitan/ot_i2c_host_proxy.h (it's still an OpenTitan implementation, but not tied to OpenTitan tool if I get it right)

hw/opentitan/ot_i2c_transport.c

AlexJones0 · 2025-10-14T10:33:18Z

hw/opentitan/ot_i2c_transport.c

+            s->parser_state = CMD_I2C_START;
+            break;
+        }
+        s->parser_state = CMD_I2C_ERR;


Maybe a warning here? "malformed byte 0x%02x received in idle state"

AlexJones0 · 2025-10-14T10:59:05Z

hw/opentitan/ot_i2c_transport.c

+    OtI2CTransportState *s = OT_I2C_TRANSPORT(opaque);
+
+    qemu_chr_fe_set_handlers(&s->chr, ot_i2c_transport_can_receive,
+                             ot_i2c_transport_receive, NULL,


Should this have an event callback to handle the cases where the CharDev is unexpectedly closed (event == CHR_EVENT_CLOSED)? Maybe also at some points we should be polling qemu_chr_fe_backend_connected and reset/error if appropriate. This might be better documented and left for a future PR however?

AlexJones0 · 2025-10-14T11:03:10Z

hw/opentitan/ot_i2c_transport.c

+    s->is_read_command = false;
+    s->address = 0u;
+    s->stall = false;
+    s->stall_timer =


I think this is not that bad if we just use the timer (and not the additional stall attribute) as I suggest in a separate comment, since the stall time is so short and we can then avoid timers causing state change across resets. But yes, maybe this should be documented as part of the lack of resets as discussed in other comments.

docs/opentitan/i2c_transport.md

AlexJones0 · 2025-10-14T11:06:01Z

docs/opentitan/i2c_transport.md

+through an externally driven `chardev` interface.
+
+The device can perform read or write transactions with bus devices and is intended to be used to
+drive the Opentitan I2C device Target mode functionality through `opentitanlib`.


Again, maybe remove reference to opentitanlib here as any host SW could use this protocol, not just upstream opentitan SW.

docs/opentitan/i2c_transport.md

The intention of this device is to be an I2C host device that is driven externally by OpenTitanTool. It will be used to issue read and write operations to other I2C devices on its bus - of primary interest is issuing commands to the OpenTitan I2C host/target block. This "bus device" is present on the bus, but it cannot be interacted with by other bus devices and cannot have transactions targetting it. Signed-off-by: Alice Ziuziakowska <[email protected]>

Signed-off-by: Alice Ziuziakowska <[email protected]>

Needed for `ot_i2c_transport` to be able to communicate with `ot_i2c` in target mode. Signed-off-by: Alice Ziuziakowska <[email protected]>

ziuziakowska requested review from AlexJones0, engdoreis and jwnrt September 18, 2025 15:28

ziuziakowska force-pushed the ot_i2c_transport branch 2 times, most recently from 4aa1651 to c990a45 Compare September 18, 2025 15:33

rivos-eblot reviewed Sep 19, 2025

View reviewed changes

ziuziakowska force-pushed the ot_i2c_transport branch from c990a45 to f759510 Compare September 19, 2025 12:38

rivos-eblot reviewed Sep 22, 2025

View reviewed changes

include/hw/opentitan/ot_i2c_transport.h Outdated Show resolved Hide resolved

ziuziakowska force-pushed the ot_i2c_transport branch 4 times, most recently from e89e6b2 to d0289a0 Compare September 23, 2025 11:43

ziuziakowska changed the title ~~[DRAFT, WIP] I2C transport device for Opentitantool~~ I2C transport device for Opentitantool Sep 23, 2025

ziuziakowska requested a review from rivos-eblot September 23, 2025 11:44

ziuziakowska force-pushed the ot_i2c_transport branch from d0289a0 to d9c2532 Compare September 23, 2025 11:48

rivos-eblot requested changes Sep 23, 2025

View reviewed changes

ziuziakowska mentioned this pull request Oct 1, 2025

[qemu] Earlgrey 1.0.0 i2c implementation lowRISC/opentitan#27896

Open

ziuziakowska force-pushed the ot_i2c_transport branch 3 times, most recently from 0e7efaa to d716e69 Compare October 3, 2025 11:11

ziuziakowska mentioned this pull request Oct 7, 2025

[sw,opentitanlib] QEMU I2C transport and target mode test lowRISC/opentitan#28450

Open

rivos-eblot reviewed Oct 7, 2025

View reviewed changes

docs/opentitan/i2c_transport.md Outdated Show resolved Hide resolved

ziuziakowska force-pushed the ot_i2c_transport branch from d716e69 to f3c8d36 Compare October 9, 2025 14:33

rivos-eblot requested changes Oct 9, 2025

View reviewed changes

hw/opentitan/ot_i2c_transport.c Show resolved Hide resolved

hw/opentitan/ot_i2c_transport.c Show resolved Hide resolved

ziuziakowska changed the title ~~I2C transport device for Opentitantool~~ I2C transport device for Opentitanlib Oct 9, 2025

ziuziakowska changed the title ~~I2C transport device for Opentitanlib~~ I2C transport device for opentitanlib Oct 9, 2025

ziuziakowska changed the title ~~I2C transport device for opentitanlib~~ ot_i2c_transport: implement device for external I2C communication Oct 10, 2025

ziuziakowska marked this pull request as ready for review October 10, 2025 09:01

rivos-eblot reviewed Oct 10, 2025

View reviewed changes

docs/opentitan/i2c_transport.md Show resolved Hide resolved

ziuziakowska requested a review from rivos-eblot October 13, 2025 15:00

rivos-eblot approved these changes Oct 14, 2025

View reviewed changes

ziuziakowska force-pushed the ot_i2c_transport branch from f3c8d36 to 1bbfd3e Compare October 14, 2025 10:25

AlexJones0 reviewed Oct 14, 2025

View reviewed changes

ziuziakowska added 2 commits October 14, 2025 16:04

[ot] hw: opentitan: add I2C transport device to Kconfig and meson

47b51e9

Signed-off-by: Alice Ziuziakowska <[email protected]>

ziuziakowska force-pushed the ot_i2c_transport branch from 1bbfd3e to 9d5ff98 Compare October 14, 2025 15:04

ziuziakowska added 2 commits October 14, 2025 16:25

[ot] docs: opentitan: document I2C transport in i2c_transport.md

6274107

Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_i2c: implement I2CSlaveClass send

c76d1bb

Needed for `ot_i2c_transport` to be able to communicate with `ot_i2c` in target mode. Signed-off-by: Alice Ziuziakowska <[email protected]>

ziuziakowska force-pushed the ot_i2c_transport branch from 9d5ff98 to c76d1bb Compare October 14, 2025 15:25


		## Protocol

		- The protocol over the chardev mirrors the I2C specification closely.

		@@ -0,0 +1,318 @@
		/*
		* OpenTitanTool I2C transport device for OpenTitan

ot_i2c_transport: implement device for external I2C communication #190

Are you sure you want to change the base?

ot_i2c_transport: implement device for external I2C communication #190

Uh oh!

Conversation

ziuziakowska commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziuziakowska commented Oct 9, 2025

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rivos-eblot commented Oct 9, 2025

Uh oh!

rivos-eblot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ziuziakowska commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rivos-eblot commented Oct 13, 2025

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziuziakowska Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

`ot_i2c_transport`: implement device for external I2C communication #190

`ot_i2c_transport`: implement device for external I2C communication #190

ziuziakowska commented Sep 18, 2025 •

edited

Loading

rivos-eblot commented Oct 10, 2025 •

edited

Loading

ziuziakowska commented Oct 13, 2025 •

edited

Loading

ziuziakowska Oct 14, 2025 •

edited

Loading

AlexJones0 Oct 14, 2025 •

edited

Loading

AlexJones0 Oct 14, 2025 •

edited

Loading